Configure Style/NumericPredicate for performance #356
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #356 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 190 190
Branches 90 90
=========================================
Hits 190 190 ☔ View full report in Codecov by Sentry. |
Replace predicate methods like `v.zero?` with direct comparison `v == 0`
for better performance.
While the current implementation may not show visible improvements,
frequently called methods using `.zero?` instead of `== 0` can lead to
significant performance losses.
Benchmarks across Ruby versions 2.5 to 3.3 consistently show comparisons
outperforming predicates:
```
Comparison (Ruby 2.6 to 3.2, v = 0):
v == 0: 21479329.3 i/s
v.zero?: 17979885.4 i/s - 1.19x (± 0.00) slower
Comparison (Ruby 2.5 and 3.3, v = 0):
v == 0: 23652215.7 i/s
v.zero?: 21843174.0 i/s - 1.08x (± 0.00) slower
Comparison (Ruby 2.5 to 3.3, v = 1):
v == 0: 23227474.2 i/s
v.zero?: 21675200.9 i/s - 1.07x (± 0.00) slower
```
tagliala
force-pushed
the
performance/configure-numeric-predicate-for-performance
branch
from
a46e568 to
50a5b0a
Compare
|
Hi @tagliala! First I want to thank you for what appears to be an awareness campaign to help the community to understand where it may be better to adjust our Rubocop configs to trade-off for performance in hot loops, as opposed to an interest in maximum readability. Thank you! Can you explain why we would make this change here, in these two lines of code? One is in benchmark set up, the other is in one-time instrumentation of an object with MemoWise. In neither case, is it yet clear to me that our trade-off would be for performance at the cost of (arguable) readability. Is that trade-off clear to you here? If so, can you help explain and make it clear? |
|
Hi,
In the specific case of memo wise, it is not going to provide any visible improvement, because as you mentioned it is a one time initialization. This is also stated in the opening post: "This will not lead to performance improvements for the current implementation of memo_wise, I'm rather submitting this as a "reminder"
It is more than clear. The fact is that this cop is globally configured for readability, so it may lead to an usage of numeric predicates in contexts where they are not optimal, like loops. Again, this is not the case. I see |
|
Thanks for making this PR, @tagliala! I appreciate the thoughtful commit message and interest in making future gem changes more performant. Personally, I'm indifferent to this change. I generally like having performant defaults, but I also struggle to see a world in which we would change this gem to include @ms-ati do you have a preference? |
|
Welcome
Thanks, and don't worry about the time. I was also proposing to understand if that method could have made the difference in some circumstances, like large codebases, or if the new default may positively have impact on future development, but as far as I understand this is not a concern for memo_wise. And if it will ever be, it will be possible to make the change in the future |
Thanks @tagliala. @JacobEvelyn my (very slight) preference would be to not merge this, sharing reasoning with what you wrote above. As you wrote above Jacob, we share a goal to encourage contributions. It sounds above as though @tagliala you are understanding that both of the maintainers in this thread absolutely do value the effort and communications you've made here. And that even if we do not merge this PR at this moment, we absolutely do encourage thoughtful and helpful contributions like this in the future! If we are all comfortable with the decision, I'd be in favor of closing this PR. |
On this point specifically I think it won't make a noticeable difference. That Let me know if I'm missing something though! |
Hello,
While the Style/NumericPredicate cop recommends using
.zero?, this can lead to performance degradation in gems where zero checks are frequent and speed-critical; for such performance-oriented libraries, using== 0remains preferable despite not adhering to the style guide.This will not lead to performance improvements for the current implementation of memo_wise, I'm rather submitting this as a "reminder"
Replace predicate methods like
v.zero?with direct comparisonv == 0for better performance.While the current implementation may not show visible improvements, frequently called methods using
.zero?instead of== 0can lead to significant performance losses.Benchmarks across Ruby versions 2.5 to 3.3 consistently show comparisons outperforming predicates:
Before merging:
README.mdand update this PRIf this change merits an update toCHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning